Add Blade ID Scan Millis Timeout #697

cshannon2 · 2024-09-29T00:38:50Z

This pull request adds a timeout to the BLADE_ID_SCAN_MILLIS option. Currently if BLADE_ID_SCAN_MILLIS is enabled it will constantly check for a blade configuration even when the lightsaber is not in use. A side effect of BLADE_ID_SCAN_MILLIS is a blinking light. The blinking light can be observed long after the lightsaber has been in use. The timeout allows the lightsaber to stop searching for a blade if it hasn't been turned on in a certain amount of time. The default time is 10 minutes. The value can be changed via a config file.

The logic:
If the blade is off and the blade has been off for longer than the timeout, then the proffie board can stop searching for a blade. BLADE_ID_SCAN_MILLIS can be resumed by turning the lightsaber on and off. Once the lightsaber is turned off the board will restart the countdown.

profezzorn · 2024-09-29T01:17:05Z

Interesting, but maybe we should just be using IDLE_OFF_TIME for this?

cshannon2 · 2024-09-29T01:33:53Z

Great Observation. I actually started this using IDLE_OFF_TIME. I was using the variable that IDLE_OFF_TIME creates which is called: last_on_time_.

The If statement logic looked like this:
if (IsOn() == false && (now - last_on_time_) < BLADE_ID_SCAN_TIMEOUT)

During testing I noticed that I was running into an issue with the BLADE_ID_SCAN_MILLIS unexpectedly restarting after a timeout had occurred. I found that it was because there is another function that modifies last_on_time_.

if (millis() - last_on_time_ > IDLE_OFF_TIME) { SaberBase::DoOff(OFF_IDLE, 0); last_on_time_ = millis(); }

The above code appeared to be resetting the timeout which resulted in restarting the BLADE_ID_SCAN_MILLIS without the lightsaber being powered on.

I wasn't sure why last_on_time_ was being reset in that function. If we can remove the last_on_time_ = millis(); from that function then I believe that I could change this to use the last_on_time_ variable.

I created BLADE_ID_SCAN_TIMEOUT instead of using IDLE_OFF_TIME to allow users to turn off the blinking light side effect without impacting other leds.

NoSloppy · 2024-09-29T02:55:19Z

blinking light side effect

By this do you mean like a single "weak" LED such as on an emitter blade PCB?

cshannon2 · 2024-09-29T03:13:23Z

@NoSloppy the short answer is yes. I have seen this side effect discussed in the forums but now I am having a hard time finding the thread. So I will just talk about my experience until I can find the thread. The emitter on my lightsaber has around 9 small leds. When there isn't a blade in the lightsaber and the lightsaber is powered on these leds provide lighting effects. In my experience when BLADE_ID_SCAN_MILLIS is enabled the 9 small leds in the emitter begin to flash / blink. This flash/blink does not occur everytime a blade_id_scan occurs and the blinking/flashing can vary in its brightness. Based off of what I have read in the forums this can be expected due to how BLADE_ID_SCAN_MILLIS functions. By setting a timeout we can prevent the blinking from occuring after the lightsaber is no longer in use. I am not sure if this change has any impact on battery life but I feel like it could potentially be beneficial for battery life.

profezzorn · 2024-09-29T07:47:00Z

props/prop_base.h

-      return true;
+    bool ScanBladeIdNow() {
+        uint32_t now = millis();
+        if (IsOn() == false && (now - blade_id_scan_start_) < BLADE_ID_SCAN_TIMEOUT) {


Why is IsOn() == false here?
That will make blade scanning while on not work properly.

The assumption was that a blade config file wouldn't change while the blade was ignited, as a result only search for a blade if the lightsaber is off and the timeout hasn't been reached. If you would like I can just remove the IsOn() check. The resulting if statement would be if ((now - blade_id_scan_start_) < BLADE_ID_SCAN_TIMEOUT)

FWIW I think shutting off BLADE_ID_SCAN_MILLIS when the blade is lit is a good idea. In my limited experiments with it, I've found that scan values can change quite significantly when the blade is lit - particularly if using SnapshotID - resulting the array constantly trying to change when you don't want it to. Indeed I would go further and add a small delay between shutting off the blade and restarting BLADE_ID_SCAN_MILLIS to ensure that unusual scan values don't cause further unwanted array switches.

@profezzorn I just wanted to bump this PR. Do you have any additional thoughts on this change?

FWIW I think shutting off BLADE_ID_SCAN_MILLIS when the blade is lit is a good idea.

Maybe, but we'd need a define for it then.

@profezzorn I just wanted to bump this PR. Do you have any additional thoughts on this change?

It is now conflicting, and cannot be merged.

@profezzorn I have resolved the conflict. I have also added a new define BLADE_ID_SCAN_WHILE_IGNITED with the default value of true. This new define will allow the user to turn off scanning when the blade is ignited. By default it will always scan the blade. The only case it will not scan for a blade is if the blade is on and BLADE_ID_SCAN_WHILE_IGNITED is defined in the user's config to equal false.

Let me know if you have any other concerns.

profezzorn · 2025-01-15T23:36:30Z

props/prop_base.h

+#define BLADE_ID_SCAN_TIMEOUT 60 * 10 * 1000
+#endif
+#ifndef BLADE_ID_SCAN_WHILE_IGNITED
+#define BLADE_ID_SCAN_WHILE_IGNITED true


This not usually how feature defines work in ProffieOS.
(They don't usually have any values, and we use #ifdef / #ifndef / defined() to test them.)

@profezzorn I refactored the way the defines where written. I believe they are identical to the other defines. Let me know if this matches the preferred style.

profezzorn · 2025-01-16T01:53:31Z

props/prop_base.h

-#ifndef BLADE_ID_SCAN_TIMEOUT
-#define BLADE_ID_SCAN_TIMEOUT 60 * 10 * 1000
+
+#ifdef BLADE_ID_SCAN_TIMEOUT


BLADE_ID_SCAN_TIMEOUT was fine (and better) before.
Please change it back.

This change has been reverted back.

Actually this #Ifdef should probably go away now. (See comments further down.)

Implemented the suggestion in the other comment.

profezzorn · 2025-01-16T01:54:14Z

props/prop_base.h

-#define BLADE_ID_SCAN_WHILE_IGNITED true
+
+#ifdef BLADE_ID_STOP_SCAN_WHILE_IGNITED
+    bool scan_while_ignited = false;


I don't want to add variables outside of the function for this.

I believe this was addressed when refactoring the scan variable.

profezzorn · 2025-01-16T01:54:38Z

props/prop_base.h

            scan = false;
        }
        else {
            scan = true;
        }
-        if (scan == true && (now - blade_id_scan_start_) < BLADE_ID_SCAN_TIMEOUT) {
+        if (scan == true && (now - blade_id_scan_start_) < scan_timeout) {


scan == true -> scan

Simplified the logic. This line should now read if (scan == true) then perform a scan. The logic to determine set the value of scan was refactored in another line of code.

profezzorn · 2025-01-16T01:56:17Z

props/prop_base.h

  bool find_blade_again_pending_ = false;
  uint32_t last_scan_id_ = 0;
    bool ScanBladeIdNow() {
        uint32_t now = millis();
        bool scan;
-        if (BLADE_ID_SCAN_WHILE_IGNITED == false && IsOn() == true){
+        if (scan_while_ignited == false && IsOn() == true){


I would recommend:
bool scan = true;
#ifdef BLADE_ID_STOP_SCAN_WHILE_IGNITED
if (IsOn()) scan=false;
#endif

This code has been implemented.

profezzorn · 2025-01-16T03:34:01Z

props/prop_base.h

+        uint32_t now = millis();
+
+        bool scan = true;
+        #ifdef BLADE_ID_STOP_SCAN_WHILE_IGNITED


Don't indent preprocessor statements, makes them harder to see.

props/prop_base.h

profezzorn · 2025-01-16T03:38:16Z

props/prop_base.h

+        }
+
+        if (scan == true) {
+            if (now - last_scan_id_ > BLADE_ID_SCAN_MILLIS) {


maybe move this up to line 596:

bool scan = (now - last_scan_id_) > BLADE_ID_SCAN_MILLIS;

Then line 607 and 608 can just be: if (scan) {

Implemented

profezzorn · 2025-01-16T03:38:27Z

props/prop_base.h

+                return true;
+            }
+            return false;
+        }


props/prop_base.h

profezzorn · 2025-01-16T03:41:47Z

props/prop_base.h

-#ifndef BLADE_ID_SCAN_TIMEOUT
-#define BLADE_ID_SCAN_TIMEOUT 60 * 10 * 1000
+
+#ifdef BLADE_ID_SCAN_TIMEOUT


Actually this #Ifdef should probably go away now. (See comments further down.)

profezzorn · 2025-01-16T04:43:58Z

props/prop_base.h

+#endif
+
+#ifdef BLADE_ID_SCAN_TIMEOUT
+#define BLADE_ID_SCAN_TIMEOUT 60 * 10 * 1000


Remove this line.

GitHub highlighted 4 different lines. Which one are you saying should be removed? Also I am not sure why any of the 4 lines highlighted would be removed.

This line:
#define BLADE_ID_SCAN_TIMEOUT 60 * 10 * 1000

It will produce a warning message that BLADE_ID_SCAN_TIMEOUT is already defined.

This has been completed. Now with the re-write / refactoring of the feature the user should be able to define a timeout if they want blade id to timeout but it won't effect users who don't specify a timeout.

profezzorn · 2025-01-16T22:50:02Z

props/prop_base.h

@@ -356,6 +356,9 @@ class PropBase : CommandParser, Looper, protected SaberBase, public ModeInterfac
  BladeSet BladeOff() {
 #ifdef IDLE_OFF_TIME
    last_on_time_ = millis();
+#endif
+#ifdef BLADE_ID_SCAN_MILLIS


shouldn't this be #ifdef BLADE_ID_SCAN_TIMEOUT ?

profezzorn · 2025-01-16T22:50:38Z

props/prop_base.h

@@ -1120,6 +1140,13 @@ class PropBase : CommandParser, Looper, protected SaberBase, public ModeInterfac

    current_mode->mode_Loop();

+#ifdef BLADE_ID_SCAN_MILLIS


same here (#ifdef BLADE_ID_SCAN_TIMEOUT)

profezzorn · 2025-01-16T22:50:56Z

props/prop_base.h

@@ -1140,6 +1167,10 @@ class PropBase : CommandParser, Looper, protected SaberBase, public ModeInterfac
 #ifdef IDLE_OFF_TIME
  uint32_t last_on_time_;
 #endif
+
+#ifdef BLADE_ID_SCAN_MILLIS


and here (#ifdef BLADE_ID_SCAN_TIMEOUT)

…D_SCAN_TIMEOUT

Add Blade ID Scan Timeout

7c6f58f

profezzorn reviewed Sep 29, 2024

View reviewed changes

cshannon2 and others added 2 commits January 15, 2025 17:35

Merge branch 'master' into SCAN_BLADE_ID_TIMEOUT

b2019e7

Added a new define to toggle blade id scan while blade is ignited

f3477f0

profezzorn reviewed Jan 15, 2025

View reviewed changes

Refactoring defines

0b46840

profezzorn reviewed Jan 16, 2025

View reviewed changes

Implementing Feedback

af79ab7

profezzorn reviewed Jan 16, 2025

View reviewed changes

Implementing Feedback

99020b8

profezzorn reviewed Jan 16, 2025

View reviewed changes

Removing Default Define for SCAN_BLADE_ID_TIMEOUT

31de15d

profezzorn reviewed Jan 16, 2025

View reviewed changes

Refactoring blade_id_scan_start_to be initiated upon defining BLADE_I…

066ca81

…D_SCAN_TIMEOUT

		@@ -1120,6 +1140,13 @@ class PropBase : CommandParser, Looper, protected SaberBase, public ModeInterfac

		current_mode->mode_Loop();

		#ifdef BLADE_ID_SCAN_MILLIS

Add Blade ID Scan Millis Timeout #697

Are you sure you want to change the base?

Add Blade ID Scan Millis Timeout #697

Conversation

cshannon2 commented Sep 29, 2024

profezzorn commented Sep 29, 2024

cshannon2 commented Sep 29, 2024 • edited Loading

NoSloppy commented Sep 29, 2024

cshannon2 commented Sep 29, 2024

Choose a reason for hiding this comment

cshannon2 Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshannon2 commented Sep 29, 2024 •

edited

Loading

cshannon2 Sep 29, 2024 •

edited

Loading